gh-136421: Load `_datetime` static types during interpreter initialization #136583

ZeroIntensity · 2025-07-12T14:07:10Z

_datetime is a special module, because it's the only non-builtin C extension that contains static types. As such, it would initialize static types in the module's execution function, which can run concurrently. Since static type initialization is not thread-safe, this caused crashes. This fixes it by moving the initialization of _datetime's static types to interpreter startup (where all other static types are initialized), which is already properly protected through other locks.

Issue: Import _datetime in sub-interpreters in the same time may crash the process #136421

I have no idea if this will fix the Windows build, but let's hope.

neonene · 2025-07-12T17:59:20Z

My concern is unnecessarily losing portability between the builtin module and the non-builtin one.

This PR avoids a crash not by locking but forcing the main interpreter initialize the _datetime static types, which I think is almost equivalent to the following change in _datetimemodule.c:

PyMODINIT_FUNC
PyInit__datetime(void)
{
+   if (PyDateTime_DateType.tp_subclasses == NULL) {
+       PyInterpreterState *interp = PyInterpreterState_Get();
+       // main interp currently checks a module before subinterp imports
+       assert(_Py_IsMainInterpreter(interp));
+       init_static_types(interp, 0);
+   }
    return PyModuleDef_Init(&datetimemodule);
}

ZeroIntensity · 2025-07-12T18:32:37Z

Hmm, that feels really hacky.

I wouldn't call the movement of _datetime to static being totally unnecessary, because _datetime exposes a C API via a capsule. If _datetime isn't included with the build of Python, things using the capsule are very likely to crash, so it makes more sense to me to require it. Are there people actually disabling _datetime in their builds?

neonene · 2025-07-12T19:25:15Z

Hmm, that feels really hacky.

The crash can be avoided even on pure Python level, and managed static extsension types are already special only for _datetime. The C API is likely to be redesigned in the future, which will be a standard way to shared extensions including _datetime.

Are there people actually disabling _datetime in their builds?

Not sure about disabling. On Windows, _datetime is a built-in module that can be switched to a DLL. I know someone who want to use such a DLL as a plugin.

ZeroIntensity · 2025-07-12T19:49:17Z

Right, there are several ways to fix this that work, but I'm trying to find the fix that is the most correct. Relying on implementation details like tp_subclasses is a little more maintenance if we ever change type initialization, and it also muddies our general rules about how static types should work. I see several options:

Make _datetime static, as it is in this PR. This makes the most sense, but I guess it could compromise compatibility in some niche cases.
Initialize static types in the PyInit_ function. This works, but will probably lead to some other issues because it mixes single and multi-phase initialization, which isn't supposed to be work.
Import _datetime in _interpreters. I really don't like this fix because it won't fix subinterpreters created via the C API.
Import _datetime in new_interpreter. This slows down initialization and is also a bit of a hack.
Make _PyStaticType_InitForExtension thread-safe. I think this will be needlessly complex.
Serialize calls to _PyStaticType_InitForExtension (gh-136421: Fix crash when _datetime is been initialized in multiple sub-interpreters #136422). This works but again hurts the story about static types, and is technically more of a bottleneck.

We have to find a balance between correctness and simplicity, and I'm not yet sure which that is.

neonene · 2025-07-12T20:33:16Z

Make _datetime static, as it is in this PR. This makes the most sense, but I guess it could compromise compatibility in some niche cases.

This is at least a feature change more than a bug fix.

ZeroIntensity · 2025-07-12T20:47:53Z

Yeah, you're right. I'd be fine with a bandaid fix for 3.14 if you want to make a PR. The PyInit_ approach probably works best, but I don't fully understand why we need the tp_subclasses check.

neonene · 2025-07-12T22:08:36Z

Still rough example, but checking tp_subclasses is an alternative to PyType_HasFeature(PyDateTime_DateType, Py_TPFLAGS_READY). I'm not sure which is better on free-threading. Both must be kept in the main interpreter in its life, which means interp_count is at least 1 in the runtime state and the subinterp shutdown cannot invoke fini_static_type() as final.

During the _datetime exec phase, the main interpreter probably needs to skip init_static_types() to not increase interp_count.

ZeroIntensity · 2025-07-13T06:56:08Z

PyType_HasFeature is definitely the better option. tp_subclasses being NULL is an implementation detail, and free-threading uses non-atomic loads for the type flags (I think it probably uses stop-the-world when modifying). People look to CPython for inspiration on their own C extensions, we should encourage them to use public APIs where possible.

aisk · 2025-07-13T07:53:03Z

A small concern is the startup time. Can you do a micro performance benchmark to measure the time cost of ./python -m 'pass' several times before and after this change? My expectation is that this will not add observable time cost.

neonene · 2025-07-13T13:33:47Z

#136620 is ready for review.

Can you do a micro performance benchmark

I haven't come up with how to measure so far.

neonene · 2025-07-14T00:19:45Z

Python/pylifecycle.c

+    status = _PyDateTime_InitTypes(interp);
+    if (_PyStatus_EXCEPTION(status)) {
+        return status;
+    }


Suggested change

status = _PyDateTime_InitTypes(interp);

if (_PyStatus_EXCEPTION(status)) {

return status;

}

if (!_Py_IsMainInterpreter(interp)) {

status = _PyDateTime_InitTypes(interp);

if (_PyStatus_EXCEPTION(status)) {

return status;

}

}

Reply from #136620 (comment)

Can you run test_concurrent_initialization() with this change? Based on the crashes that come from the change, I said this PR and my example are "almost equivalent." I'm not sure right now what this PR ensures.

Hm, what are you trying to achieve here? This will just break the types for the main interpreter.

This will just break the types for the main interpreter.

Note that test_concurrent_initialization() does not load the _datetime in the main inter interpreter at all.

Correction: Run the script of the test without running test_datetime.

ZeroIntensity · 2025-07-19T23:53:24Z

After discussion on DPO, the consensus is that we are okay with making _datetime static, as long as we fix the capsule in the process: https://discuss.python.org/t/datetime-should-be-a-static-module/98857.

@vstinner, would you mind doing a review?

kumaraditya303 · 2025-07-20T04:18:54Z

Modules/_datetimemodule.c

@@ -7347,11 +7344,11 @@ init_static_types(PyInterpreterState *interp, int reloading)
    for (size_t i = 0; i < Py_ARRAY_LENGTH(capi_types); i++) {
        PyTypeObject *type = capi_types[i];
        if (_PyStaticType_InitForExtension(interp, type) < 0) {
-            return -1;


Not for this PR but as a follow up I think it would be better to now remove _PyStaticType_InitForExtension and just use _PyStaticType_InitBuiltin for it, there's a lot of special casing that could be removed.

kumaraditya303 · 2025-07-20T04:25:55Z

Modules/_datetimemodule.c

@@ -7379,10 +7376,6 @@ _datetime_exec(PyObject *module)
    }
    /* We actually set the "current" module right before a successful return. */

-    if (init_static_types(interp, reloading) < 0) {


I haven't look at the history of this code in detail but is the reloading logic still necessary or can we remove it from get_current_module?

It looks like it's still used by some macros:

cpython/Modules/_datetimemodule.c

Line 7406 in 0ad304f

if (!reloading) {

I think the macros use the reloading logic to primarily avoid modifying the static types after is initialised once multiple times, you can probably move that to _PyDateTime_InitTypes now and remove it from module exec, it makes sense to have all modifications done to static types done as part of runtime init rather than module init.

kumaraditya303 · 2025-07-20T05:03:24Z

Lib/test/datetimetester.py

@@ -3651,6 +3651,25 @@ def test_repr_subclass(self):
        td = SubclassDatetime(2010, 10, 2, second=3)
        self.assertEqual(repr(td), "SubclassDatetime(2010, 10, 2, 0, 0, 3)")

+    @support.cpython_only
+    def test_concurrent_initialization(self):


Let's rename this to test_concurrent_initialization_subinterpreters and add another test where datetime is already imported in main interp and then imported in subinterp

neonene · 2025-07-20T12:55:53Z

Lib/test/datetimetester.py

+    @support.cpython_only
+    def test_concurrent_initialization_subinterpreter(self):
+        # Run in a subprocess to ensure we get a clean version of _datetime


Please put an anchor instead of the well-known explanation of assert_python_ok(). Also, move the test to ExtensionModuleTests (@support.cpython_only is redundant there). TestDateTime is the place to test the datetime class.

Sorry, I'm not sure what you mean by "an anchor".

Sorry about that. I meant the gh-issue number or the url.

Ok, did both.

neonene · 2025-07-20T12:56:18Z

Lib/test/datetimetester.py

+            print('a', end='')
+
+        with InterpreterPoolExecutor() as executor:
+            for _ in range(8):


It's up to you, but range(10) is probably better than range(8) for the Windows x64 CI to reproduce the issue.

Eh, I think this is fine. Many other systems have 8 cores, not 10.

Okay, but it sounds like you are talking about the max_workers argument rather than the submit count here.

max_workers just chooses the number of threads on the system when not set. We could try to submit os.cpu_count() number of futures, but we don't need to overcomplicate this; we just need something that stresses several subinterpreters trying to import _datetime concurrently.

neonene · 2025-07-20T12:56:37Z

Modules/_datetimemodule.c

-
    // `&...` is not a constant expression according to a strict reading
    // of C standards. Fill tp_base at run-time rather than statically.
    // See https://bugs.python.org/issue40777


This PR does not address the possible races in PyDateTime_*.tp_base = &PyDateTime_*Type; below, right?

I guess not. I don't think there's an easy way to do this here, because atomically storing tp_base will continue to race with all the non-atomic reads elsewhere.

Do we even need to load it at runtime like this? We have other examples of directly storing it in the PyTypeObject structure:

cpython/Objects/methodobject.c

Line 396 in 958657b

.tp_base = &PyCFunction_Type,

It is not needed now because the module is statically linked, that issue happens only with dynamic loaded modules so you can define it statically now.

I guess a _Py_IsMainInterPreter() check will suffice in this PR?

It is not needed now because the module is statically linked, that issue happens only with dynamic loaded modules so you can define it statically now.

Ah, TIL. That's definitely the best option here then.

I guess a _Py_IsMainInterPreter() check will suffice in this PR?

For what?

For ensuring tp_base is set only once after Py_Initialize(). But I missed the Kumar 's comment.

Please remove PyDateTime_DateTimeType.tp_base = &PyDateTime_DateType; as well.

This reverts commit 6adafa1.

ZeroIntensity added 4 commits July 12, 2025 09:47

Load _datetime during interpreter initialization

77d117c

Add a test case.

963a9ee

Add blurb.

e16fb54

Move to pycore_init_types()

ed65682

ZeroIntensity requested a review from aisk July 12, 2025 14:07

ZeroIntensity requested review from pganssle, abalkin, ericsnowcurrently and erlend-aasland as code owners July 12, 2025 14:07

ZeroIntensity added the topic-subinterpreters label Jul 12, 2025

bedevere-app bot added the awaiting core review label Jul 12, 2025

bedevere-app bot mentioned this pull request Jul 12, 2025

Import _datetime in sub-interpreters in the same time may crash the process #136421

Open

ZeroIntensity mentioned this pull request Jul 12, 2025

gh-136421: Fix crash when _datetime is been initialized in multiple sub-interpreters #136422

Open

ZeroIntensity added 2 commits July 12, 2025 10:12

Fix lint.

43b4843

Add _datetime to the frozen modules.

0ad304f

I have no idea if this will fix the Windows build, but let's hope.

ZeroIntensity requested a review from a team as a code owner July 12, 2025 14:22

aisk approved these changes Jul 12, 2025

View reviewed changes

neonene reviewed Jul 14, 2025

View reviewed changes

neonene mentioned this pull request Jul 14, 2025

gh-136421: Fix crash in _datetime when initialized/finalized concurrently #136620

Closed

kumaraditya303 reviewed Jul 20, 2025

View reviewed changes

Remove obsolete 'reloading' state.

d762ed5

kumaraditya303 reviewed Jul 20, 2025

View reviewed changes

ZeroIntensity added 2 commits July 20, 2025 01:14

Some test refactoring and improvements.

db327e7

Fix test.

9456147

neonene reviewed Jul 20, 2025

View reviewed changes

ZeroIntensity added 6 commits July 20, 2025 09:56

Make tp_base stores atomic.

6adafa1

Revert "Make tp_base stores atomic."

6e2f891

This reverts commit 6adafa1.

Set tp_base in the PyTypeObject definition.

a15843f

Add a comment.

d6064c4

Move the test.

9a3b1c2

Remove dead tp_base code.

3520514

Uh oh!

gh-136421: Load _datetime static types during interpreter initialization #136583

Are you sure you want to change the base?

gh-136421: Load _datetime static types during interpreter initialization #136583

Conversation

ZeroIntensity commented Jul 12, 2025 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neonene commented Jul 12, 2025

Uh oh!

ZeroIntensity commented Jul 12, 2025

Uh oh!

neonene commented Jul 12, 2025

Uh oh!

ZeroIntensity commented Jul 12, 2025

Uh oh!

neonene commented Jul 12, 2025

Uh oh!

ZeroIntensity commented Jul 12, 2025

Uh oh!

neonene commented Jul 12, 2025

Uh oh!

ZeroIntensity commented Jul 13, 2025

Uh oh!

aisk commented Jul 13, 2025

Uh oh!

neonene commented Jul 13, 2025

Uh oh!

neonene Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neonene Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity commented Jul 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity Jul 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gh-136421: Load `_datetime` static types during interpreter initialization #136583

gh-136421: Load `_datetime` static types during interpreter initialization #136583

ZeroIntensity commented Jul 12, 2025 •

edited by bedevere-app bot

Loading

neonene Jul 14, 2025 •

edited

Loading

neonene Jul 14, 2025 •

edited

Loading

ZeroIntensity Jul 20, 2025 •

edited

Loading